Fix missing module in large app from Experimental Bundler #8303

AGawrys · 2022-07-12T22:33:52Z

This PR addresses a missing module where only shared bundles were considered when attempting to remove a shared bundle from a group.

When determining what source bundles need the shared-bundle-to-remove's assets pushed back into it. Instead of consulting the total BundleGroup bundles, we consulted only shared bundles. This structure would then always be empty, and so shared bundles were removed from groups but not placed into source bundles at all.

Major Changes

We've removed sharedToSourceBundles (A structure that held shared bundle ids to source bundle ids)
We now handle the case where if a shared bundle is removed from a Bundle Group, and only used elsewhere once, it should just be fully removed
We now consider reused bundles in parallel request limit removal
Refactor reused bundle code (we attempt to reuse bundles before creating a shared bundle if a bundle is the exact subgraph of some other set of bundles)

Reused Bundles

Reused bundles are bundles that are shared (i.e. they have source bundles) but also have a mainEntryAsset. These bundles have a referencing bundle (i.e. something that imports it) but are also exact subgraphs of other bundles. So, they can be shared, but they should also never be removed by parallel request limits since they have a referencing bundles (something that dynamically imported it, causing a new bundle to be created) (See future todo for plans to remove these as an optimization)

Diagrams

In this bundleGraph,

Bar.js requires a.js and b.js (and requires Foo.js through a.js)
Foo.js requires a.js and b.js
a.js requires foo.js

Thus, foo.js is a subgraph of bar.js and creating a shared bundle would be wrong since it would be an exact copy of the foo bundle and leave an empty shell bundle behind.

✔️ PR Todo

Added/updated unit tests for this change
Filled out test instructions (In case there aren't any unit tests)
Test on large app

Future TODO

Remove sourceBundle property from bundles in favor of reading graph
Remove a reused bundle if it has been removed by parallel request limit (currently this is not done because reused bundle have some referencing or parent bundle as it's main reason for being a bundle. However, that bundle may be small and better suited to simply be merged back in)
[FIXED] Potential error: What if a reused bundle has a shared bundle with some other bundle. Then, if that bundle group hits the parallel request limit, and is merged back in, only the reused bundles' assets would be merged in and the shared bundle is now not connected

Shared Bundle under Reused Bundle Case Diagram

…et, add tests for removal based on limit and size

…under parallel request limit

parcel-benchmark · 2022-07-12T23:15:11Z

Benchmark Results

Kitchen Sink ✅

Timings

Description	Time	Difference
Cold	1.50s	+6.00ms
Cached	344.00ms	+7.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

React HackerNews ✅

Timings

Description	Time	Difference
Cold	9.14s	+126.00ms
Cached	444.00ms	-11.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

AtlasKit Editor ✅

Timings

Description	Time	Difference
Cold	1.61m	-4.00ms
Cached	2.50s	-45.00ms

Cold Bundles

Bundle	Size	Difference	Time	Difference
dist/index.html	240.00b	+0.00b	1.21m	+33.32s ⚠️

Cached Bundles

Bundle	Size	Difference	Time	Difference
dist/esm.c7dc1640.js	61.96kb	+0.00b	1.20m	+31.79s ⚠️
dist/workerHasher.e50d242f.js	1.72kb	+0.00b	1.20m	+31.79s ⚠️
dist/16.1969624f.js	1.08kb	+0.00b	38.95s	+2.40s ⚠️
dist/index.html	240.00b	+0.00b	1.20m	+36.87s ⚠️

Three.js ✅

Timings

Description	Time	Difference
Cold	6.67s	+82.00ms
Cached	272.00ms	-1.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

Click here to view a detailed benchmark overview.

devongovett

A few questions, but lets get this in so we can test

packages/bundlers/experimental/src/ExperimentalBundler.js

devongovett · 2022-07-26T15:50:29Z

packages/bundlers/experimental/src/ExperimentalBundler.js

+        invariant(reusableBundle !== 'root' && reusableBundle != null);
+        reusableBundle.sourceBundles.add(candidateSourceBundleId);
+      } else {
+        // Asset is not a bundleRoot, but if its parent bundle can be


Only the direct parent? Is there any case where we'd have to do this recursively?

So actually I think the comment isn't the best. Essentially what we're trying to do here is:

Of the reachable assets for an asset, is there some common subset (under a bundleRoot) that may be reused.
So I guess it's not so much as a parent, as it is any bundleRoot that synchronously imports the asset in question. (which to me is kind of a parent? Just not the only parent)

So I guess the better way to phrase would be "if a bundleRoot of a bundle that this asset is in, can be reused by some other bundleRoot in the asset's reachable, reuse it"

And it's important to note, this code path you're talking about was only added because we can't guarantee that we process a bundleRoot before any non-bundleRoot assets that are in it, when placing assets into bundles.
So like if we process a non-bundleRoot asset, we need to make sure we don't prematurely place it in, where it could've been reused by some other bundleRoot

Diagrams for this section might be important 😄 ..working on it

* upstream/v2: (22 commits) Cross compile toolchains are built into docker image already Also fix release build Update centos node version v2.7.0 Changelog for v2.7.0 Use placeholder expression when replacing unused symbols (#8358) Lint (#8359) Add support for errorRecovery option in @parcel/transformer-css (#8352) VS Code Extension for Parcel (#8139) Add multi module compilation for elm (#8076) Bump terser from 5.7.2 to 5.14.2 (#8322) Bump node-forge from 1.2.1 to 1.3.0 (#8271) allow cjs config files on type module projects (#8253) inject script for hmr when there is only normal script in html (#8330) feat: support react refresh for @emotion/react (#8205) Update index.d.ts (#8293) remove charset from jsloader script set (#8346) Log resolved targets in verbose log level (#8254) Fix missing module in large app from Experimental Bundler (#8303) [Symbol Propagation] Non-deterministic bundle hashes (#8212) ...

AGawrys added 5 commits July 12, 2022 10:42

consider reused bundles as shared bundles, turn source bundles into s…

ef84fc6

…et, add tests for removal based on limit and size

do not consider reuse bundles for min size removal as it breaks build

74e8719

rename reusing of bundles step and de fork test

655a1aa

Handle case where rused bundle pulls in shared and then gets removed …

7a06961

…under parallel request limit

add to source bundles for shared bundle

b90f7de

AGawrys requested review from devongovett, mischnic, gorakong and thebriando July 12, 2022 22:33

Merge branch 'v2' into remove-reused-shared-bundle-under-limit

b682bb3

AGawrys added 3 commits July 13, 2022 12:55

lint

9cffae0

disable test for default due to varied functionality

c3180b2

oops exclusive tests

23b277d

devongovett mentioned this pull request Jul 19, 2022

[Experimental Bundler]: Implement parallel request limits #7863

Closed

break out of sourceBundle iteration after a bundle is removed

dedbdb6

devongovett approved these changes Jul 26, 2022

View reviewed changes

AGawrys added 2 commits July 27, 2022 12:49

Merge branch 'v2' into remove-reused-shared-bundle-under-limit

dc2c879

fix comment

a1f80a0

AGawrys merged commit 70b2abc into v2 Jul 27, 2022

AGawrys deleted the remove-reused-shared-bundle-under-limit branch July 28, 2022 19:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix missing module in large app from Experimental Bundler #8303

Fix missing module in large app from Experimental Bundler #8303

AGawrys commented Jul 12, 2022

parcel-benchmark commented Jul 12, 2022 •

edited

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

devongovett left a comment

devongovett Jul 26, 2022

AGawrys Jul 27, 2022

AGawrys Jul 27, 2022

AGawrys Jul 27, 2022 •

edited

Fix missing module in large app from Experimental Bundler #8303

Fix missing module in large app from Experimental Bundler #8303

Conversation

AGawrys commented Jul 12, 2022

Major Changes

Reused Bundles

Diagrams

✔️ PR Todo

Future TODO

Shared Bundle under Reused Bundle Case Diagram

parcel-benchmark commented Jul 12, 2022 • edited

Benchmark Results

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

devongovett left a comment

Choose a reason for hiding this comment

devongovett Jul 26, 2022

Choose a reason for hiding this comment

AGawrys Jul 27, 2022

Choose a reason for hiding this comment

AGawrys Jul 27, 2022

Choose a reason for hiding this comment

AGawrys Jul 27, 2022 • edited

Choose a reason for hiding this comment

parcel-benchmark commented Jul 12, 2022 •

edited

AGawrys Jul 27, 2022 •

edited